fix(#153): Ghost objects removed from the in-memory python list of the parent#160
Conversation
|
|
||
| assert len(reverted_car.parts) == 0 | ||
|
|
||
| with pytest.raises(IndexError): |
There was a problem hiding this comment.
assert len(reverted_car.parts) == 0 should be enough?
you can try doing self.session.flush() and check that it should not raise KeyError (to reproduce original error mentioned in commit msg).
There was a problem hiding this comment.
assert len(reverted_car.parts) == 0should be enough?you can try doing
self.session.flush()and check that it should not raiseKeyError(to reproduce original error mentioned in commit msg).
Thank You Ashish for nudging me in the right direction. So essentially the key error was still happening when I tried to flush, so just removing the deleted objects from the in-memory python list wasn't enough. So what I did now is essentially ensure that if something is not in memory and it is already marked for deletion then it is never lazy loaded, and the record just marks it with None. This actually matches the pattern of how we handle non-polymorphic objects, when we can't safely read a value from a deleted object.
Please let me know if I need to make any more changes,
Regards,
Anubhav
|
Great Debugging Skills! Some suggestion for commit structuring:
|
Anubhav-2003
force-pushed
the
fix-issue-153
branch
from
4a9d670 to
1217f94
Compare
Thank You Ashish for the kind words, I have squashed the minor syntax commit to the original one, and also marked the second commit with the same fix() structure as I did with the first one. It really helps, when being guided, so thank you again. Please let me know if I need to make any more changes. Regards, |
ashish16052
left a comment
ashish16052
left a comment
There was a problem hiding this comment.
LGTM, let me check why CI isn't running - and then we can merge this.
Thank You, Ashish. |
|
@Anubhav-2003 I've stabilized CI in main branch. Could you please rebase your changes on top of that? Let's get this merged once the CI is green 🚀 |
…t of the parent When we were reverting a relationship where a child object was marked for deletion, it was not being removed from the Python's in-memory data-structure (collection). This left 'ghost' objects in the collection. If an autoflush occurred, SQLAlchemy attempted to lazy-load deferred columns for these ghost objects, causing a KeyError: Deferred loader failed crash, which was especially prevalent if we had a polymorphic sub-class. This fix ensures the items are actively removed from the collection list in memory using collection.remove(value), keeping the Python state perfectly synced with the database session state.
Anubhav-2003
force-pushed
the
fix-issue-153
branch
from
1217f94 to
456f88d
Compare
Hi Ashwin, I have rebased my changes on top of the new main branch changes. Please let me know if I need to change anything else. Thank You. Regards, |
… the plugin from improperly triggering lazy-loads on deleted rows.
Anubhav-2003
force-pushed
the
fix-issue-153
branch
from
456f88d to
d5de7c8
Compare
Sorry for the linting error. I fixed it and merged it with the existing two commits. |
3 participants
When we were reverting a relationship where a child object was marked for deletion, it was not being removed from the Python's in-memory data-structure (collection). This left 'ghost' objects in the collection. If an autoFlush occurred, SQLAlchemy attempted to lazy-load deferred columns for these ghost objects, causing a KeyError: Deferred loader failed crash, which was especially prevalent if we had a polymorphic sub-class. This fix ensures the items are actively removed from the collection list in memory using collection.remove(value), keeping the Python state perfectly synced with the database session state.